Skip to content

Fix require components depth #19976

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Closed
wants to merge 9 commits into from
Closed

Fix require components depth #19976

wants to merge 9 commits into from

Conversation

SkiFire13
Copy link
Contributor

Note: I suggest reviewing commit-by-commit

Objective

Solution

  • Update required components registration to be oblivious of the recursive registration happening.
  • Concretely this means removing the requiree, inheritance_depth and recursion_check_stack parameters from Component::register_required_components
    • requiree was effectively unused
    • inheritance_depth is now useless, because the depth is always either 0 (for direct requirements) or computed in register_inherited_required_components by bumping the depth of the inherited components
    • recursion_check_stack has been moved to ComponentsRegistrator, which also allows to make enforce_no_required_components_recursion private.

Testing

@SkiFire13
Copy link
Contributor Author

Putting this into draft mode since after a bit more analysis this seems to be the wrong order (it's breath-first-like/lower-inheritance-depth-preferred but should be depth-first instead)

@SkiFire13 SkiFire13 marked this pull request as draft July 6, 2025 15:30
@alice-i-cecile alice-i-cecile added C-Bug An unexpected or incorrect behavior A-ECS Entities, components, systems, and events C-Code-Quality A section of code that is hard to understand or change S-Needs-Review Needs reviewer attention (from anyone!) to move forward S-Waiting-on-Author The author needs to make changes or address concerns before this can be merged and removed S-Needs-Review Needs reviewer attention (from anyone!) to move forward labels Jul 6, 2025
@alice-i-cecile
Copy link
Member

Putting this into draft mode since after a bit more analysis this seems to be the wrong order (it's breath-first-like/lower-inheritance-depth-preferred but should be depth-first instead)

Good catch. We should add more tests to ensure that this is caught.

@SkiFire13
Copy link
Contributor Author

Good catch. We should add more tests to ensure that this is caught.

The issue is that we also have features that assume a breath-first-like/lower-inheritance-depth-preferred order, and we have tests for them too

@SkiFire13 SkiFire13 closed this Jul 13, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
A-ECS Entities, components, systems, and events C-Bug An unexpected or incorrect behavior C-Code-Quality A section of code that is hard to understand or change S-Waiting-on-Author The author needs to make changes or address concerns before this can be merged
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

RequiredComponent.inheritance_depth docs do not matchComponent derive impl
2 participants
@SkiFire13 @alice-i-cecile